Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ulimited body parsing #1983

Merged
merged 4 commits into from
Oct 27, 2023
Merged

Fix ulimited body parsing #1983

merged 4 commits into from
Oct 27, 2023

Conversation

const-t
Copy link
Contributor

@const-t const-t commented Sep 27, 2023

If the response doesn't have body tempesta parses the body until the connection is closed, in this case need to update overall body size and don't make chunks.

-Added error handling to tfw_cache_h2_copy_body()

@const-t
Copy link
Contributor Author

const-t commented Sep 28, 2023

@krizhanovsky Is this condition correct? If so tfw_apm_hm_srv_alive() will be called only for suspended server.

Copy link
Contributor

@krizhanovsky krizhanovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have only one question and if there is no problem, then just comment why do we exepct body in the first skb and merge.

Regarding tfw_http_hm_control() - we return from the function if the server is alive (not suspended) since we do not need to mark it alive with tfw_srv_mark_alive(), so the code seems correct. This is an example why it's bad to write code without comments :)

it->frag = -1;

/* Set starting position. */
r = ss_skb_find_frag_by_offset(it->skb, body_start, &it->frag);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we expect to find start of the body in the first skb?

Copy link
Contributor Author

@const-t const-t Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not first skb. tfw_body_iter_init() - Initializes iterator with data obtained during body parsing, we pass to this function pointer to start of the body and pointer to sk_buff that holds the body or first chunks of the body. We use these pointers as starting point. If skb start doesn't "contain" passed pointer body_start error will be handled below. ss_skb_find_frag_by_offset() finds index of the fragment that contains body_start pointer.

@const-t
Copy link
Contributor Author

const-t commented Oct 16, 2023

Regarding tfw_http_hm_control() - we return from the function if the server is alive (not suspended) since we do not need to mark it alive with tfw_srv_mark_alive(), so the code seems correct. This is an example why it's bad to write code without comments :)

This part is ok. However looks strange following condition if we take into account that tfw_apm_hm_srv_alive() must be called for alive server at least once to calculate valid crc32 in case when directive resp_crc32 is auto:

if (!tfw_srv_suspended(srv) ||
	    !tfw_apm_hm_srv_alive(resp->status, &resp->body, srv->apmref))

/*
* Special case for 'auto' monitor: generate crc32
* from body of first response and store it into monitor.
*/
if (!hm->crc32 && hm->auto_crc) {
hm->crc32 = tfw_str_crc32_calc(body);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tfw_str_crc32_calc is now unused, we can remove it from our code and tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer don't remove this function. Time to time we use crc32 for TfwStr even for debug purposes and it useful to have such function.

If the response doesn't have body tempesta parses the body until the connection
is closed, in this case need to update overall body size and don't make
chunks.

-Added error handling to `tfw_cache_h2_copy_body()`
There is copule of places where need to iterate over http message body.
For this purposes has been added body iterator. Just initialize
iterator and chunk using `tfw_body_iter_init()` with error handling.
Use `TFW_BODY_ITER_WALK` macro for walking over the body as TfwStr.

tfw_apm_hm_srv_alive(): Calculate crc32 for message body using body
itertor instead of looping invalid TfwStr.
@const-t const-t merged commit 1000722 into master Oct 27, 2023
1 check passed
@const-t const-t deleted the kt-1868-fix branch October 27, 2023 08:58
@enuribekov-tempesta enuribekov-tempesta mentioned this pull request May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kernel panic when cache is enabled and server responds with tcp segmentation
3 participants